Cache idf_versions.json on build time so we have target selection availible in case of offline installation #282
Conversation
Summary of ChangesHello @Hahihula, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and offline capabilities of the application by implementing a build-time caching mechanism for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a caching mechanism for idf_versions.json by embedding it at compile time. This is a great improvement for offline installations, making the tool more robust. The implementation correctly falls back to the cached version when the online version cannot be fetched.
My review includes two main points:
- A suggestion to avoid duplicating the URL for
idf_versions.json, improving maintainability. - A note on the newly added
include_url_macrodependency, which brings in an older version ofreqwest, potentially increasing binary size. An alternative is suggested.
Overall, this is a valuable change that enhances the offline capabilities of the installer.
src-tauri/Cargo.toml
Outdated
| rustpython-vm = { git = "https://github.com/Hahihula/RustPython.git", branch = "test-rust-build", features = ["freeze-stdlib"], optional = true } | ||
| rustpython-stdlib = { git = "https://github.com/Hahihula/RustPython.git", branch = "test-rust-build", features = ["ssl-vendor"], optional = true } | ||
| os_info = "3.12.0" | ||
| include_url_macro = "0.1.0" |
There was a problem hiding this comment.
The include_url_macro crate was last updated 3 years ago and depends on an older version of reqwest (0.11.x), while the project uses a newer version (0.12.x). This will result in both versions of reqwest and their respective dependencies being compiled into the binary, increasing its size. While Cargo handles this, it's worth considering if there's an alternative macro that is more up-to-date. For example, the include_bytes_from_url crate is more recent and could be a good alternative. It would require a small change to use serde_json::from_slice instead of from_str.
| // TODO: handle the possibility of multiple downloads | ||
| pub async fn get_idf_versions() -> Result<Releases, String> { | ||
| Ok(download_idf_versions().await.unwrap()) | ||
| let cached_idf_versions = include_url!("https://dl.espressif.com/dl/esp-idf/idf_versions.json"); |
There was a problem hiding this comment.
The URL is hardcoded here, which duplicates the IDF_VERSIONS_URL constant defined on line 40. This could lead to inconsistencies if the URL needs to be updated. Since include_url! requires a string literal, you can't use the constant directly. A good practice to avoid duplication is to define the URL in a macro and use it in both places.
First, you could change the constant definition to this:
macro_rules! idf_versions_url {
() => {
"https://dl.espressif.com/dl/esp-idf/idf_versions.json"
};
}
pub const IDF_VERSIONS_URL: &str = idf_versions_url!();Then, you can use the macro here as suggested.
| let cached_idf_versions = include_url!("https://dl.espressif.com/dl/esp-idf/idf_versions.json"); | |
| let cached_idf_versions = include_url!(idf_versions_url!()); |
Hahihula
force-pushed
the
EIM-370-do-not-crash-without-internet-connection
branch
2 times, most recently
from
712ee39 to
7662ed7
Compare
…lible in case of offline installation
Hahihula
force-pushed
the
EIM-370-do-not-crash-without-internet-connection
branch
from
7662ed7 to
c579c48
Compare
No description provided.